i made this a separate block because once disable_account_loader_special_case activates it is simply deleted. we will also be able to remove program_cache and program_accounts from AccountLoader and they become free-floating variables in transaction_processor.rs again

also an idea i have been toying with after we do that is to simply implement TransactionProcessingCallback for AccountLoader itself so we can pass it to program cache setup, which will happen after account loading. this depends on how feasible it is to restrict callbacks to a concrete type (unless we just make two traits)

while i was getting this pr ready, i realized we dont need to replace zero lamport accounts in the account cache with AccountSharedData::default() at all, so that saves us some Arc::new() calls. we just need something in there that stops us from going to accounts-db, and we can replace it when reading. account_cache is private and load_account() is the only function that reads from it, so we dont need to handle them special anywhere else

i left the cache insert unguarded by disable_account_loader_special_case which means it will be used by 2.2 the moment this pr lands. this is safe (@jstarry should confirm):

• there is no change to pre-simd83 consensus if disable_account_loader_special_case is not active, because the account cache comes after the program cache, so behavior is identical to if these accounts came from accounts-db
• disable_account_loader_special_case is in 2.0, so it is almost guaranteed to activate before the simd83 consensus gate which will relax account locks. but if, for some startling reason, simd83 consensus activates first, it is still fine. there is an edge case where the program cache would shadow the account cache if you close and reopen a loaderv3 buffer. but this only applies if the account is loaded invisible, so it only affects transaction data size, and this is fine because simd83 consensus necessarily can only be activated by its own feature gate

after this pr lands, we can contemplate a more restricted interface for passing only this account through. maybe slot_history: Option<AccountSharedData> on the transaction processing environment

bit of an aside, but the sol_get_sysvar syscall we added in 2.0 for core bpf program conversion does not support SlotHistory by design, we only needed StakeHistory and SlotHashes. so the good news is it doesnt introduce a mechanism to bypass this override

the possibly better news is... if neither me nor joe needed to add SlotHistory, do any important bpf programs or builtins actually use it? i have no idea. maybe we dont need to let programs see this sysvar at all if its only used by the validator itself. if so, we could add some #[deprecated] where needed and specialcase it like the instructions sysvar in load_transaction_account to return an account with no data (would require a feature gate, perhaps we could avoid a simd)

the test account is read-only, but this is now necessary because svm would put it in the account cache the first time it loads it in an entry and then assume it had been deallocated the second time it loads it. the svm behavior is correct, because there should be no way to bring an account to zero lamports without it being writable, therefore we should never be able to see one in a read-only context in the first place. our builtins and sysvars have dust lamports

I thought there was a discussion on previous PR about this inspected_as_writable stuff.
The function that is called to inspect accounts already has guards against doing it twice.

Is this still necessary?

we never got confirmation on that. ill check with brooks

alright i have clarity on this now, the thing i was missing is that the idea svm has of "what things to inspect" and the idea bank has of "what results of inspection do i actually care about" are disjoint concepts. we can get rid of the state tracking and inspect every time we get an account from accounts-db or account cache. bank has its own dedupe logic and other implementers of inspect can do whatever they want

To avoid allocating every time, we can change to use thread local variable instead, wdyt.
example:

thread_local! {
    static HAS_DUPLICATES_SET: RefCell<AHashSet<Pubkey>> = RefCell::new(AHashSet::with_capacity(MAX_TX_ACCOUNT_LOCKS));
}

this code is just getting a reasonable upper bound on account cache size. the mechanism in your pr to prevent duplicate transactions in svm isnt required because svm has no concept of duplicate transactions. my understanding is theyre presently implicitly deduped at the account locks level by relying on the fee-payer lock and we just need to do this explicitly with message hashes instead

I am not suggesting here to de-duplicate transactions within a batch. The purpose of this example is to point out a thread_local variable use case to avoid the allocation over-head for account cache in every batch.

Regarding the de-duplication of transactions, when conflicting batches will be allowed the account locking function won't implicitly de-duplicate such transactions. Then as you said we need to explicitly check a batch does not have any duplicate transactions using message hashes.

oh, ty for the suggestion but we dont want to reuse the cache between batches

Should we add a debug assert that the size of account_overrides is equal to 1 if sysvar is present and 0 otherwise? Or is it basically impossible now that we decreased the vis of the account overrides set_account function?

it isnt possible, the only exposed setter is set_slot_history now (which accepts AccountSharedData and hardcodes the Pubkey as sysvar::slot_history::id(). we can remove AccountOverrides entirely now as a cleanup. im thinking ill look into it when i start on simd186 since id like to change something about simulation results, and this is related to simulation only

Shouldn't we check if lamports is 0 before saying the account is alive? Otherwise you could read a dead account into the cache and later write to it and even tho it's still dead, we say it's alive.

youre right, ill open a pr for this on monday. thankfully it doesnt affect bank because to get here we would have already had to inspect as writable (which is why this was missed; before removing AccountCacheItem, inspecting a previously-alive now-dead account never happened at all)

edit: actually there is a slight edge case of taking a dead account read-only, leaving it dead, and making it alive, but this is only possible post-simd83 and the fix is the same

edit2: nevermind there actually is no way to get a bad first inspect because to bring the account to zero lamports in the first place it must necessarily be inspected as writable

Hmm isn't there a bug post simd-83 where you read a dead account in one tx and then write to it in another? The inspect for the write tx would say it's alive when it's actually dead

yes, i made my first edit thinking of that, but made the second edit thinking of the reason why we didnt need to check for equality with AccountSharedData::default(). but in this case the account doesnt need to exist

(i have a pr for this now but have been blocked by ci build issues, will add you once its green)